spanset: assert that batches don't access store local and unreplicated RangeID local keys #157153
Conversation
|
This change is |
iskettaneh
force-pushed
the
rse3
branch
2 times, most recently
from
9f7f91b to
ed7f5cd
Compare
iskettaneh
force-pushed
the
rse3
branch
2 times, most recently
from
4d115e0 to
4fbc791
Compare
Potential Bug(s) DetectedThe three-stage Claude Code analysis has identified potential bug(s) in this PR that may warrant investigation. Next Steps: Note: When viewing the workflow output, scroll to the bottom to find the Final Analysis Summary. After you review the findings, please tag the issue as follows:
|
github-actions
bot
added
the
o-AI-Review-Potential-Issue-Detected
Potential Bug(s) DetectedThe three-stage Claude Code analysis has identified potential bug(s) in this PR that may warrant investigation. Next Steps: Note: When viewing the workflow output, scroll to the bottom to find the Final Analysis Summary. After you review the findings, please tag the issue as follows:
|
iskettaneh
force-pushed
the
rse3
branch
2 times, most recently
from
f9daad1 to
cdb4edc
Compare
pkg/kv/kvserver/spanset/spanset.go
Outdated
| forbiddenSpansMatchers []func(roachpb.Span) error | ||
| allowUndeclared bool | ||
| allowForbidden bool |
There was a problem hiding this comment.
Pre-existing, but made stronger in this PR: it seems that this type shouldn't mix the job of representing a set of spans and that of [test-only] verifying it / forbidding keys. There seems to be a clear subset of methods that add/canonicalize/access spans without having an opinion on them, and then the "check" methods that verify stuff. I would make the latter the responsibility of a wrapper (either existing one like spanSetBatch, or make a new one).
Ideally need a prereq PR/commit that does that separation.
There was a problem hiding this comment.
Will do it in another PR
iskettaneh
force-pushed
the
rse3
branch
2 times, most recently
from
6f5f0a9 to
8193ffc
Compare
iskettaneh
left a comment
iskettaneh
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @pav-kv)
pkg/kv/kvserver/spanset/spanset.go
Outdated
| forbiddenSpansMatchers []func(roachpb.Span) error | ||
| allowUndeclared bool | ||
| allowForbidden bool |
There was a problem hiding this comment.
Will do it in another PR
pav-kv
left a comment
pav-kv
left a comment
There was a problem hiding this comment.
This is nearing the LGTM, thanks for making it cleaner each time.
iskettaneh
left a comment
iskettaneh
left a comment
There was a problem hiding this comment.
Reviewable status:
iskettaneh
left a comment
iskettaneh
left a comment
There was a problem hiding this comment.
Reviewable status:
iskettaneh
left a comment
iskettaneh
left a comment
There was a problem hiding this comment.
Reviewable status:
pav-kv
left a comment
pav-kv
left a comment
There was a problem hiding this comment.
The non-test parts LGTM, minor suggestions. Request to clean up the tests a bit, so that I can make a final (self-convincing) pass on them and LGTM tomorrow.
pkg/kv/kvserver/spanset/batch.go
Outdated
| // TODO(ibrahim): The fields spans, spansOnly, and ts don't seem to be used. | ||
| // Consider removing them and performing the necessary clean ups. |
There was a problem hiding this comment.
or making them used as intended
pav-kv
left a comment
pav-kv
left a comment
There was a problem hiding this comment.
LGTM, thanks for this change! Some last suggestions included, apply as many as makes sense.
|
Thank you for the detailed review Pavel! bors r+ |
157153: spanset: assert that batches don't access store local and unreplicated RangeID local keys r=iskettaneh a=iskettaneh This PR adds the following test-only assertions: 1) Generated batches don't touch store-local keys. 2) Generated batches don't touch unreplicated RangeID local keys. We disable the check in exactly 3 locations we know that we currently touch those keys. Fixes: #156537 Release note: None Co-authored-by: iskettaneh <[email protected]>
|
Build failed: |
This commit introduces the concept of forbidden spans, which will allow us to assert our batches don't modify Raft's engine keys.
Analogous to how we have a spanset helper contains() that understands the special span representation: [x-eps,x). This commit adds Overlaps that expects the same span representation. Moreover, this commit makes this special span representation explicit by introducing a new type called `TrickySpan`.
…d RangeID local keys This commit adds the following test-only assertions: 1) Generated batches don't touch store-local keys. 2) Generated batches don't touch unreplicated RangeID local keys. We disable the check in exactly 2 locations we know that we currently touch those keys.
|
bors r+ |
3 participants
This PR adds the following test-only assertions:
We disable the check in exactly 3 locations we know that we currently
touch those keys.
Fixes: #156537
Release note: None